-
-
Notifications
You must be signed in to change notification settings - Fork 558
Feature/mcp client initialized event #277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e event immediately after currentClient is populated with actual values\n- Event now includes correct client_name and client_version in analytics payload\n- Added client_info_available property to track edge cases where clientInfo is missing\n- Resolves timing issue where run_server_start captured 'uninitialized' client name
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA telemetry capture event ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| deferLog('info', `Client connected: ${currentClient.name} v${currentClient.version}`); | ||
| } | ||
|
|
||
| capture('mcp_client_initialized'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Include the current client name/version when emitting the initialization analytics event. [enhancement]
Severity Level: Minor
| capture('mcp_client_initialized'); | |
| capture('mcp_client_initialized', { | |
| client: { | |
| name: currentClient?.name || 'unknown', | |
| version: currentClient?.version || 'unknown' | |
| } | |
| }); |
Why it matters? ⭐
The suggestion is valid and low-risk: adding client name/version to the initialization event gives richer analytics and is straightforward to implement.
Note: src/utils/capture.ts already already enriches events with client context (it reads and injects currentClient -> client_name/client_version), so this change will duplicate that data in the event payload rather than fix a bug. Duplication isn't harmful because captureBase sanitizes and filters properties before sending, but it means the improvement is mainly cosmetic/analytics-related (better explicitness) rather than required to make telemetry work.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/server.ts
**Line:** 134:134
**Comment:**
*Enhancement: Include the current client name/version when emitting the initialization analytics event.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server.ts (1)
134-135: LGTM! Telemetry addition is well-placed.The
mcp_client_initializedevent is correctly placed after client initialization setup and within error handling. The fire-and-forget pattern (not awaiting) is acceptable.Optional improvements for consistency:
- Consider awaiting the capture call to ensure telemetry is sent before the response returns, matching the pattern used for other successful operations (e.g., lines 1066, 1087, 1106):
- capture('mcp_client_initialized'); + await capture('mcp_client_initialized');
- Consider including client metadata as properties for richer analytics (ensuring no PII is included):
- await capture('mcp_client_initialized'); + await capture('mcp_client_initialized', { + client_name: currentClient.name, + has_version: !!currentClient.version + });These are minor consistency improvements rather than issues—the current implementation works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.
📚 Learning: 2025-05-25T16:02:52.184Z
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.
Applied to files:
src/server.ts
🧬 Code graph analysis (1)
src/server.ts (1)
src/utils/capture.ts (1)
capture(277-284)
CodeAnt-AI Description
Emit mcp_client_initialized event after client info is set
What Changed
mcp_client_initializedanalytics event immediately after a connecting client's name and version are recorded during initializationImpact
✅ Accurate client name/version in startup analytics✅ Fewer analytics events labeled "uninitialized"✅ Clearer client-version data for telemetry and diagnostics💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit